do not return 404 when using query params or trailing slash #2893
Conversation
🦋 Changeset detectedLatest commit: 7aba56b The changes in this PR will be included in the next version bump. This PR includes changesets to release 10 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Let's hold this one. That "if" condition is important for the performance. Maybe the path needs to be normalized in whatwg-node instead |
|
@nescalante What is the use-case of using a trailing slash for the graphql endpoint? Why would you want to use both In case you want to only use |
|
@n1ru4l basically in general sites "ignore" the trailing slash so both https://github.com/dotansimha/graphql-yoga/ and github.com/dotansimha/graphql-yoga are valid urls. Express also supports both URL with trailing and without trailing slash. |
|
@ardatan regarding performance, would you prefer this alternative without regexes? |
|
@nescalante Wouldn't that indicate duplicated routes, shouldn't a trailing slash instead return a 301 with the location without the trailing slash? |
|
ok, two questions then:
if you agree with approach 1, then I will be happy to discuss the right way to provide this option. My main concern is that validations over URL are too strict, and I can't handle them. So I can't have graphql over two endpoints for example, or graphql in one endpoint, and graphiql in another one |
|
😴 |
| graphiql: () => Promise.resolve({ title: 'Test GraphiQL' }), | ||
| }) | ||
| const response = await yoga.fetch( | ||
| 'http://localhost:3000/graphql?query=something+awesome', |
There was a problem hiding this comment.
Could you add tests for trailing slash as well?
There was a problem hiding this comment.
closing this PR, not interested anymore
|
Thank you @nescalante for the contribution and I'm very sorry we haven't got to review it in a timely manner... We've now shipped this change thanks to another contribution from @antonio-iodice We'll do better with swiftly reviewing changes from valuable contributors such as yourself |
this PR fixes two errors:
I added a test for these two cases and then fixed them